Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

benchmark contract execution #436

Merged
merged 5 commits into from
Aug 30, 2021
Merged

benchmark contract execution #436

merged 5 commits into from
Aug 30, 2021

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Aug 15, 2021 •
edited
Loading

Closes: #431

Description

$ go test ./x/evm/keeper -v -run="^$" -bench=".*"
BenchmarkTokenTransfer
BenchmarkTokenTransfer-16    	    2996	    347253 ns/op	  119528 B/op	    1756 allocs/op
BenchmarkEmitLogs
BenchmarkEmitLogs-16         	       1	1071863815 ns/op	1515836464 B/op	13601697 allocs/op

it seems we do have some significant performance issue with log emit. takes a second to emit 1000 logs.

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@netlify
Copy link

netlify bot commented Aug 15, 2021 •
edited
Loading

✔️ Deploy Preview for stoic-ritchie-0f4a47 ready!

🔨 Explore the source changes: 2f5fbd0

🔍 Inspect the deploy log: https://app.netlify.com/sites/stoic-ritchie-0f4a47/deploys/6119db5c8e7ae10008fab37c

😎 Browse the preview: https://deploy-preview-436--stoic-ritchie-0f4a47.netlify.app

@yihuang yihuang marked this pull request as draft August 16, 2021 02:45
@yihuang yihuang marked this pull request as ready for review August 16, 2021 03:30
@JayT106 JayT106 mentioned this pull request Aug 19, 2021
Merged
11 tasks
@leejw51crypto
Copy link
Contributor

how about the speed without logs?

fedekunze
fedekunze reviewed Aug 25, 2021
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK. I would add more comments to the code. Also, thoughts of using ReportAlloc()?

x/evm/keeper/keeper_test.go Outdated
@@ -50,20 +80,22 @@ type KeeperTestSuite struct {

appCodec codec.Codec
signer keyring.Signer

EvmDenom string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can obtain this from the EVM module params

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed now, changed to a method.

x/evm/keeper/keeper_test.go
@@ -146,6 +183,46 @@ func (suite *KeeperTestSuite) initKeepersWithmAccPerms() (authkeeper.AccountKeep
return authKeeper, keeper
}

// DeployTestContract deploy a test erc20 contract and returns the contract address
func (suite *KeeperTestSuite) DeployTestContract(t require.TestingT, owner common.Address, supply *big.Int) common.Address {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious of why we are using require.TestingT instead of testing.T

Copy link
Contributor Author

@yihuang yihuang Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it work on both testing.B and testing.T.

Added a line of comment to explain.

x/evm/keeper/keeper_test.go
}

func (suite *KeeperTestSuite) SetupTest() {
func (suite *KeeperTestSuite) DoSetupTest(t require.TestingT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, can you add a comment on why you are using require.TestingT instead of just making this the setup function as before?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also on why you are using require.XXX(t, ...) instead of suite.Require(). Otherwise someone without the context could revert the changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it work on both testing.B and testing.T.

Added a line of comment to explain.

x/evm/keeper/benchevm_test.go
b.ResetTimer()
b.StartTimer()
for i := 0; i < b.N; i++ {
ctx, _ := suite.ctx.CacheContext()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why cache context here?

Copy link
Contributor Author

@yihuang yihuang Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make each run independent, we don't want to mutate the shared storage for each runs.

@fedekunze
Copy link
Contributor

@yihuang I will merge this PR, but can you address the comments from the review in a follow-up PR?

@fedekunze fedekunze enabled auto-merge (squash) August 26, 2021 15:51
@fedekunze fedekunze mentioned this pull request Aug 26, 2021
Closed
@orijbot
Copy link

orijbot commented Aug 28, 2021

Visit https://dashboard.github.orijtech.com?back=0&pr=436&remote=true&repo=yihuang%2Fethermint to see benchmark details.

@fedekunze
Copy link
Contributor

Seems that some tests are failing

@fedekunze fedekunze merged commit a5ab608 into evmos:main Aug 30, 2021
@yihuang yihuang deleted the benchmark branch September 2, 2021 05:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fedekunze fedekunze fedekunze left review comments

@khoslaventures khoslaventures Awaiting requested review from khoslaventures

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

evm transaction execution is not benchmarked
4 participants
@yihuang @leejw51crypto @fedekunze @orijbot